Skip to content

fix(extensions): guard non-numeric catalog downloads in search/info rendering#3710

Open
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/extension-downloads-non-numeric-guard
Open

fix(extensions): guard non-numeric catalog downloads in search/info rendering#3710
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/extension-downloads-non-numeric-guard

Conversation

@jawwad-ali

@jawwad-ali jawwad-ali commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Problem

specify extension search and specify extension info <id> render a catalog entry's downloads field with the :, thousands separator:

stats.append(f"Downloads: {ext['downloads']:,}")   # extension_search
...
stats.append(f"Downloads: {ext_info['downloads']:,}")  # _print_extension_info

guarded only by is not None. But :, is only valid for int/float — on a string it raises ValueError: Cannot specify ',' with 's'.

Catalog payloads are only shape-validated (_validate_catalog_payload checks that extensions is a dict); individual entry fields are never type-checked, and _get_merged_extensions returns the raw catalog dicts. So a catalog entry with "downloads": "1500" (a quoted number — realistic from a community catalog, a SPECKIT_CATALOG_URL catalog, a project/user catalog, or a poisoned cache) makes the whole command abort with an uncaught traceback. Reproduced end-to-end: with such a cache, both extension search and extension info exit non-zero with ValueError("Cannot specify ',' with 's'.").

Fix

Group-format downloads only when it is actually numeric; otherwise render it as-is:

downloads = ext.get('downloads')
if downloads is not None:
    stats.append(
        f"Downloads: {downloads:,}"
        if isinstance(downloads, (int, float))
        else f"Downloads: {downloads}"
    )

(and the equivalent in _print_extension_info). Numeric values (int/float, including bool) take the identical :, branch, so correct catalogs render byte-for-byte the same — only the previously-crashing case changes to print the value. This mirrors the module's existing defensive treatment: every other field in these two renderers is already str()-wrapped.

Verification

  • test_info_renders_non_numeric_downloads (direct) and test_search_survives_non_numeric_downloads (end-to-end via CliRunner): both fail before with the exact ValueError, pass after (exit_code == 0, output shows Downloads: 1500).
  • TestExtensionCatalog: 64 passed. ruff clean.

AI-assisted: authored with Claude Code. I reproduced the crash on main through the real CLI (catalog cache with a string downloads) before writing the guard, and confirmed numeric output is unchanged.

@jawwad-ali @claude
fix(extensions): guard non-numeric catalog downloads in search/info r…
9fba052 
…endering

`specify extension search` and `specify extension info <id>` format a catalog
entry's `downloads` field with the `:,` thousands separator, guarded only by
`is not None`. Catalog payloads are only shape-validated -- individual fields
are never type-checked and `_get_merged_extensions` returns raw catalog dicts
-- so an entry with a non-numeric `downloads` (e.g. the JSON string "1500",
realistic from a community / SPECKIT_CATALOG_URL / project catalog) makes the
`:,` format raise `ValueError: Cannot specify ',' with 's'`, aborting the
whole command with an uncaught traceback.

Group-format `downloads` only when it is actually numeric; otherwise render it
as-is. Numeric values (int/float, incl. bool) format identically, so correct
catalogs are byte-for-byte unchanged. Every other field in these two renderers
is already `str()`-wrapped; this closes the one unguarded field.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 24, 2026 08:55
@mnriem
mnriem requested a review from Copilot July 24, 2026 20:35
Copilot AI reviewed Jul 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Guards extension catalog rendering against non-numeric download counts.

Changes:

  • Formats numeric downloads with separators and renders other values directly.
  • Adds coverage for search and info rendering.
Show a summary per file
File Description
src/specify_cli/extensions/_commands.py Adds defensive download rendering.
tests/test_extensions.py Tests string-valued downloads.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/specify_cli/extensions/_commands.py:991

  • The info renderer also passes the untrusted fallback directly to Rich. For example, downloads="[/red]foo" still produces an uncaught MarkupError, while valid markup can inject styling or links. Escape the string fallback so arbitrary non-numeric catalog values actually render safely.
        stats.append(
            f"Downloads: {downloads:,}"
            if isinstance(downloads, (int, float))
            else f"Downloads: {downloads}"
        )
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread src/specify_cli/extensions/_commands.py
@jawwad-ali @claude
fix(extensions): escape the non-numeric downloads fallback for Rich m…
135444f 
…arkup

Address review feedback: the fallback interpolated the untrusted catalog value
straight into a Rich-rendered string, so guarding the ``:,`` ValueError just
traded it for a MarkupError -- a catalog entry with downloads "[/red]foo" still
aborted `extension search`/`info`, and balanced tags could restyle the output.

Wrap the fallback in _escape_markup(str(...)) at both sites, matching how every
other catalog field in these renderers is already escaped. Numeric values keep
the identical ``:,`` branch, so correct catalogs are unchanged.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali

jawwad-ali commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

You're right — fixed in 135444f. Guarding the :, ValueError just traded it for a MarkupError: I confirmed Console().print("Downloads: [/red]foo") raises closing tag '[/red]' doesn't match any open tag, so extension search/info still aborted on a hostile catalog value.

Both sites now escape the fallback — f"Downloads: {_escape_markup(str(downloads))}" — matching how every other catalog field in these renderers is already handled. The numeric branch keeps the identical :, format, so correct catalogs render byte-for-byte the same.

Tests parametrized over "1500", "[/red]foo" (unbalanced → MarkupError) and "[bold]x[/bold]" (balanced → would silently restyle); the two markup cases fail before this commit and pass after.

@jawwad-ali @claude
fix(extensions): escape 'stars' too, in the same stats string
348d0ee 
Follow-up to the downloads escaping: `stars` is the other catalog-controlled
value joined into the same Rich-rendered stats line, and it was still raw --
verified that stars "[/red]x" raises the same MarkupError and aborts
`extension info`/`search`. Hardening one of the two adjacent values would have
left the reported defect reachable through the sibling field.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@mnriem mnriem Awaiting requested review from mnriem mnriem is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jawwad-ali @mnriem